[EuiBreadcrumbs] Add `popoverContent` and `popoverProps` #7031

cee-chen · 2023-08-03T00:48:08Z

Summary

Per the above issue, Kibana's new serverless design needs the ability to allow breadcrumbs to toggle popovers that can act as more complex navigation patterns:

This PR also updates the collapsed ... breadcrumb to dogfood the new popover API and (hopefully) increases a11y UX for it as well. As always, I recommend following along by commit as there's a few incidental cleanups.

QA

Go to https://eui.elastic.co/pr_7031/#/navigation/breadcrumbs#popover-content
Confirm the demo renders breadcrumbs that trigger popovers, and the content within the popovers look correct
Regression testing
- In the "Responsive" demo on the page, confirm the ... collapsed breadcrumb's popover behavior is the same as on production

General checklist

Checked in both light and dark modes
Checked in mobile
- Mobile collapsed behavior isn't super great because you end up in a popover-within-a-popover situation, but I'm not super sure there's any way around this, and at least it works?
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs (using @default if default values are missing) and playground toggles
Added documentation
Added or updated jest ~~and cypress tests~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Updated the Figma library counterpart~~
~~- [ ] Checked Code Sandbox works for any docs examples~~

- for readability/consistency - swap EuiLink & EuiTextColor order

+ name conditional more clearly + pull out color logic and add some missing tests for `highlightLastBreadcrumb`

- note that `title` change in snapshot is not actually correct and only occurs because of EuiIcon testenv logic

cee-chen · 2023-08-03T01:05:30Z

src/components/breadcrumbs/breadcrumb.tsx

+     * If passed, both `href` and `onClick` will be ignored - the breadcrumb's
+     * click behavior should only trigger a popover.
+     */
+    popoverContent?: ReactNode;


I'm actually not 100% sold on this prop name, but couldn't really think of anything better. Open to suggestions if folks are also not a fan or have other ideas!

If we weren't typing these props, I might suggest changing it, but having ReactNode next to it and the TS errors if folks get confused, I think this is a good prop name.

cee-chen · 2023-08-03T01:10:05Z

src/components/breadcrumbs/breadcrumb.tsx

+  const ariaCurrent = highlightLastBreadcrumb ? ('page' as const) : undefined;
+
+  const isPopoverBreadcrumb = !!popoverContent;
+  const [isPopoverOpen, setIsPopoverOpen] = useState(false);


Just wanted to mention, I know this may feel a little odd technically to have a state that only 1 out of 3 types of breadcrumbs use, but I figure it isn't a huge deal (at least perf-wise) since it's not causing rerenders or similar for breadcrumbs that do not have popovers.

That being said if the entire architecture of this component feels janky to folks (I def have a hunch it could be cleaner / there's some amount of code smell going on here), I'm open to proposals! Or if we just want to call it good in terms of effort vs. reward I'm also fine with that 🤷

I'm all for simple components (a personal preference) but the logic to determine types of breadcrumbs is easy to trace, so this feels okay to keep it collected.

cee-chen · 2023-08-03T01:12:01Z

src/components/breadcrumbs/breadcrumb.tsx

+  const popoverAriaLabel = useEuiI18n(
+    'euiBreadcrumb.popoverAriaLabel',
+    'Clicking this button will toggle a popover dialog.'
+  );


@1Copenut I'd super appreciate your screen reader testing on this and letting me know if you think this sounds natural to SR users!

This does sound natural and gave me a clear cue that this was no ordinary breadcrumb. I tested with VO and NVDA (Chrome + FF) and all three announced the breadcrumb name, node type, and helper message. Was a really nice experience.

cee-chen · 2023-08-03T01:12:39Z

src/components/breadcrumbs/breadcrumb.test.tsx

+  describe('highlightLastBreadcrumb', () => {
+    it('adds an aria-current attr', () => {


These tests are not related to any logic added in this PR, but the entire breadcrumb file was missing tests so I figured I'd just add some branch coverage while I was here 🥲

kibanamachine · 2023-08-03T01:14:35Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7031/

kibanamachine · 2023-08-03T01:15:39Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7031_buildkite/

1Copenut

👍 LGTM! I left comments about the screen reader text and my thought on the useState. One suggestion for the comment text, use or discard as you see fit. Thanks @cee-chen !

src/components/breadcrumbs/breadcrumb.tsx

1Copenut · 2023-08-03T16:14:06Z

src/components/breadcrumbs/breadcrumb.tsx

+     * If passed, both `href` and `onClick` will be ignored - the breadcrumb's
+     * click behavior should only trigger a popover.
+     */
+    popoverContent?: ReactNode;


If we weren't typing these props, I might suggest changing it, but having ReactNode next to it and the TS errors if folks get confused, I think this is a good prop name.

1Copenut · 2023-08-03T16:30:08Z

src/components/breadcrumbs/breadcrumb.tsx

+  const ariaCurrent = highlightLastBreadcrumb ? ('page' as const) : undefined;
+
+  const isPopoverBreadcrumb = !!popoverContent;
+  const [isPopoverOpen, setIsPopoverOpen] = useState(false);


I'm all for simple components (a personal preference) but the logic to determine types of breadcrumbs is easy to trace, so this feels okay to keep it collected.

1Copenut · 2023-08-03T16:31:10Z

src/components/breadcrumbs/breadcrumb.tsx

+  const popoverAriaLabel = useEuiI18n(
+    'euiBreadcrumb.popoverAriaLabel',
+    'Clicking this button will toggle a popover dialog.'
+  );


This does sound natural and gave me a clear cue that this was no ordinary breadcrumb. I tested with VO and NVDA (Chrome + FF) and all three announced the breadcrumb name, node type, and helper message. Was a really nice experience.

1Copenut · 2023-08-03T16:32:23Z

src/components/breadcrumbs/breadcrumb.tsx

+          css: cssStyles,
+        };
+
+        if (isPopoverBreadcrumb) {


This if / else if / else goes to what I mentioned in my previous comment. The logic is easy to trace back to props and it's very clear what will be rendered based on what's being passed.

Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>

kibanamachine · 2023-08-03T17:26:40Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7031_buildkite/

kibanamachine · 2023-08-03T17:28:06Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_7031/

elasticmachine · 2023-08-03T17:39:40Z

💚 Build Succeeded

Buildkite Build
Commit: f582787

History

💚 Build #174 succeeded 97033e5

`v86.0.0`⏩`v87.1.0` ⚠️ The biggest set of type changes in this PR come from the breaking change that makes `pageSize` and `pageSizeOptions` now optional props for `EuiBasicTable.pagination`, `EuiInMemoryTable.pagination` and `EuiDataGrid.pagination`. This caused several other components that were cloning EUI's pagination type to start throwing type warnings about `pageSize` being optional. Where I came across these errors, I modified the extended types to require `pageSize`. These types and their usages may end up changing again in any case once the Shared UX team looks into #56406. --- ## [`87.1.0`](https://github.com/elastic/eui/tree/v87.1.0) - Updated the underlying library powering `EuiAutoSizer`. This primarily affects typing around the `disableHeight` and `disableWidth` props ([#6798](elastic/eui#6798)) - Added new `EuiAutoSize`, `EuiAutoSizeHorizontal`, and `EuiAutoSizeVertical` types to support `EuiAutoSizer`'s now-stricter typing ([#6798](elastic/eui#6798)) - Updated `EuiDatePickerRange` to support `compressed` display ([#7058](elastic/eui#7058)) - Updated `EuiFlyoutBody` with a new `scrollableTabIndex` prop ([#7061](elastic/eui#7061)) - Added a new `panelMinWidth` prop to `EuiInputPopover` ([#7071](elastic/eui#7071)) - Added a new `inputPopoverProps` prop for `EuiRange`s and `EuiDualRange`s with `showInput="inputWithPopover"` set ([#7082](elastic/eui#7082)) **Bug fixes** - Fixed `EuiToolTip` overriding instead of merging its `aria-describedby` tooltip ID with any existing `aria-describedby`s ([#7055](elastic/eui#7055)) - Fixed `EuiSuperDatePicker`'s `compressed` display ([#7058](elastic/eui#7058)) - Fixed `EuiAccordion` to remove tabbable children from sequential keyboard navigation when the accordion is closed ([#7064](elastic/eui#7064)) - Fixed `EuiFlyout`s to accept custom `aria-describedby` IDs ([#7065](elastic/eui#7065)) **Accessibility** - Removed the default `dialog` role and `tabIndex` from push `EuiFlyout`s. Push flyouts, compared to overlay flyouts, require manual accessibility management. ([#7065](elastic/eui#7065)) ## [`87.0.0`](https://github.com/elastic/eui/tree/v87.0.0) - Added beta `componentDefaults` prop to `EuiProvider`, which will allow configuring certain default props globally. This list of components and defaults is still under consideration. ([#6923](elastic/eui#6923)) - `EuiPortal`'s `insert` prop can now be configured globally via `EuiProvider.componentDefaults` ([#6941](elastic/eui#6941)) - `EuiFocusTrap`'s `crossFrame` and `gapMode` props can now be configured globally via `EuiProvider.componentDefaults` ([#6942](elastic/eui#6942)) - `EuiTablePagination`'s `itemsPerPage`, `itemsPerPageOptions`, and `showPerPageOptions` props can now be configured globally via `EuiProvider.componentDefaults` ([#6951](elastic/eui#6951)) - `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid` now allow `pagination.pageSize` to be undefined. If undefined, `pageSize` defaults to `EuiTablePagination`'s `itemsPerPage` component default. ([#6993](elastic/eui#6993)) - `EuiBasicTable`, `EuiInMemoryTable`, and `EuiDataGrid`'s `pagination.pageSizeOptions` will now fall back to `EuiTablePagination`'s `itemsPerPageOptions` component default. ([#6993](elastic/eui#6993)) - Updated `EuiHeaderLinks`'s `gutterSize` spacings ([#7005](elastic/eui#7005)) - Updated `EuiHeaderAlert`'s stacking styles ([#7005](elastic/eui#7005)) - Added `toolTipProps` to `EuiListGroupItem` that allows customizing item tooltips. ([#7018](elastic/eui#7018)) - Updated `EuiBreadcrumbs` to support breadcrumbs that toggle popovers via `popoverContent` and `popoverProps` ([#7031](elastic/eui#7031)) - Improved the contrast ratio of disabled titles within `EuiSteps` and `EuiStepsHorizontal` to meet WCAG AA guidelines. ([#7032](elastic/eui#7032)) - Updated `EuiSteps` and `EuiStepsHorizontal` to highlight and provide a more clear visual indication of the current step ([#7048](elastic/eui#7048)) **Bug fixes** - Single uses of `<EuiHeaderSectionItem side="right" />` now align right as expected without needing a previous `side="left"` sibling. ([#7005](elastic/eui#7005)) - `EuiPageTemplate` now correctly displays `panelled={true}` ([#7044](elastic/eui#7044)) **Breaking changes** - `EuiTablePagination`'s default `itemsPerPage` is now `10` (was previously `50`). This can be configured through `EuiProvider.componentDefaults`. ([#6993](elastic/eui#6993)) - `EuiTablePagination`'s default `itemsPerPageOptions` is now `[10, 25, 50]` (was previously `[10, 20, 50, 100]`). This can be configured through `EuiProvider.componentDefaults`. ([#6993](elastic/eui#6993)) - Removed `border` prop from `EuiHeaderSectionItem` (unused since Amsterdam theme) ([#7005](elastic/eui#7005)) - Removed `borders` object configuration from `EuiHeader.sections` ([#7005](elastic/eui#7005)) **CSS-in-JS conversions** - Converted `EuiHeaderAlert` to Emotion; Removed unused `.euiHeaderAlert__dismiss` CSS ([#7005](elastic/eui#7005)) - Converted `EuiHeaderSection`, `EuiHeaderSectionItem`, and `EuiHeaderSectionItemButton` to Emotion ([#7005](elastic/eui#7005)) - Converted `EuiHeaderLinks` and `EuiHeaderLink` to Emotion; Removed `$euiHeaderLinksGutterSizes` Sass variables ([#7005](elastic/eui#7005)) - Removed `$euiHeaderBackgroundColor` Sass variable; use `$euiColorEmptyShade` instead ([#7005](elastic/eui#7005)) - Removed `$euiHeaderChildSize` Sass variable; use `$euiSizeXXL` instead ([#7005](elastic/eui#7005)) --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>

cee-chen added 6 commits August 2, 2023 17:22

[EuiBreadcrumb] Add popoverContent and popoverProps APIs

1364fec

[cleanup] Convert return to a single if/elseif/if branch

ed64b7b

- for readability/consistency - swap EuiLink & EuiTextColor order

[cleanup] DRY out repeated props between each breadcrumb type

ca1dc21

+ name conditional more clearly + pull out color logic and add some missing tests for `highlightLastBreadcrumb`

Update EuiBreadcrumbCollapsed to dogfood new popoverContent prop

b0a28db

[a11y] Screen reader enhancement

b638dfb

- note that `title` change in snapshot is not actually correct and only occurs because of EuiIcon testenv logic

[docs] Add docs example for new prop

dc60dde

cee-chen requested a review from 1Copenut August 3, 2023 00:48

changelog

97033e5

cee-chen requested a review from a team August 3, 2023 01:04

cee-chen commented Aug 3, 2023

View reviewed changes

1Copenut approved these changes Aug 3, 2023

View reviewed changes

[PR feedback] i accidentally a word

f582787

Co-authored-by: Trevor Pierce <1Copenut@users.noreply.github.com>

cee-chen enabled auto-merge (squash) August 3, 2023 17:17

cee-chen merged commit 375e9e5 into elastic:main Aug 3, 2023
1 check passed

cee-chen deleted the serverless-breadcrumbs branch August 3, 2023 17:54

cee-chen mentioned this pull request Aug 15, 2023

Upgrade EUI to v87.1.0 elastic/kibana#163961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiBreadcrumbs] Add `popoverContent` and `popoverProps` #7031

[EuiBreadcrumbs] Add `popoverContent` and `popoverProps` #7031

cee-chen commented Aug 3, 2023 •

edited

Loading

cee-chen Aug 3, 2023 •

edited

Loading

1Copenut Aug 3, 2023

cee-chen Aug 3, 2023

1Copenut Aug 3, 2023

cee-chen Aug 3, 2023

1Copenut Aug 3, 2023

cee-chen Aug 3, 2023

kibanamachine commented Aug 3, 2023

kibanamachine commented Aug 3, 2023

1Copenut left a comment

1Copenut Aug 3, 2023

1Copenut Aug 3, 2023

1Copenut Aug 3, 2023

1Copenut Aug 3, 2023

kibanamachine commented Aug 3, 2023

kibanamachine commented Aug 3, 2023

elasticmachine commented Aug 3, 2023

		describe('highlightLastBreadcrumb', () => {
		it('adds an aria-current attr', () => {

[EuiBreadcrumbs] Add popoverContent and popoverProps #7031

[EuiBreadcrumbs] Add popoverContent and popoverProps #7031

Conversation

cee-chen commented Aug 3, 2023 • edited Loading

Summary

QA

General checklist

cee-chen Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 3, 2023

kibanamachine commented Aug 3, 2023

1Copenut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 3, 2023

kibanamachine commented Aug 3, 2023

elasticmachine commented Aug 3, 2023

💚 Build Succeeded

History

[EuiBreadcrumbs] Add `popoverContent` and `popoverProps` #7031

[EuiBreadcrumbs] Add `popoverContent` and `popoverProps` #7031

cee-chen commented Aug 3, 2023 •

edited

Loading

cee-chen Aug 3, 2023 •

edited

Loading